Fix: stop FileHandler after error responses#5530
Conversation
Signed-off-by: sonjungchan <rnrmfjc@gmail.com>
Signed-off-by: sonjungchan <rnrmfjc@gmail.com>
Signed-off-by: sonjungchan <rnrmfjc@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes FileHandler (/file/:key) so it stops processing immediately after writing an error response, preventing additional work (e.g., DB lookups, Referer parsing, manifest generation) and avoiding multiple unrelated errors being appended after the response is committed.
Changes:
- Add early
returnstatements after each error response inFileHandler. - Replace raw HTTP status codes with
net/httpstatus constants. - Add a regression test ensuring an invalid token does not trigger downstream DB access or additional error messages.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| chaoscenter/graphql/server/pkg/handlers/file_handler.go | Ensures handler returns immediately after writing any error response; uses http.Status* constants. |
| chaoscenter/graphql/server/pkg/handlers/handlers_test.go | Adds a regression test to confirm invalid token errors short-circuit without DB calls or follow-on errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kunalworldwide
left a comment
There was a problem hiding this comment.
Good catch — the missing return statements after error responses meant the handler would fall through and try subsequent operations with invalid state (e.g., calling GetInfra after JWT validation failed, or parsing a URL from an empty referrer). This could lead to confusing cascaded errors in logs and potentially writing multiple response bodies.
The fix is straightforward: add return after each error response. The switch from magic numbers (500, 200) to http.StatusInternalServerError / http.StatusOK is a nice cleanup too.
The test verifies the key behavior: an invalid JWT token should produce a 500 response with the JWT error message and NOT proceed to call MongoDB or parse headers. The AssertNotCalled on the mock operator confirms the early return works.
One small observation: the err.Error() written to the response body on JWT validation failure could leak internal details to the client (token parsing error messages). For a production API you'd typically want a generic error message in the response and the detailed error only in logs. Not a regression from this PR though — it was already the pattern before.
LGTM.
Proposed changes
FileHandlernow returns immediately after token validation, infra lookup, Referer parsing, or manifest generation errors. This prevents/file/:keyrequests from doing extra work or appending unrelated errors after the response is already committed.Fixes #5529
Testing
Before this fix, the new regression test fails because
FileHandlercontinues after an invalid token and callsMongoOperator.Get.After this fix:
Result:
Types of changes
Checklist
Dependency
None.